Skip to content

gh-140650: fix SystemError in io.BufferedWriter.close when closed errors #140653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

gh-140650: fix SystemError in io.BufferedWriter.close when closed errors #140653

wants to merge 7 commits into from
+43 −6

Conversation

@inventshah
Copy link
Contributor

@inventshah inventshah commented Oct 27, 2025 •
edited by bedevere-app bot
Loading

Restores the 3.14 behavior of emitting a TypeError. Preservation of the sign helps in the error checking here:

cpython/Modules/_io/bufferedio.c

Lines 556 to 563 in 3dab11f

/* gh-138720: Use IS_CLOSED to match flush CHECK_CLOSED. */
r = IS_CLOSED(self);
if (r < 0)
goto end;
if (r > 0) {
res = Py_NewRef(Py_None);
goto end;
}

db68bfc changed from buffered_closed to IS_CLOSED.

@bedevere-app bedevere-app bot mentioned this pull request Oct 27, 2025
Open
@cmaloney cmaloney self-requested a review October 27, 2025 05:36
ashm-dev
ashm-dev suggested changes Oct 27, 2025
View reviewed changes
serhiy-storchaka
serhiy-storchaka reviewed Oct 27, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look right. If IS_CLOSED() returns a positive value and sets an error, there is a bug in IS_CLOSED().

@cmaloney
Copy link
Contributor

cmaloney commented Oct 27, 2025

@serhiy-storchaka : Overall problem is code assumes IS_CLOSED can't result in an exception being set but if a user-provided implementation does unusual things (ex. closed = NotImplemented) then that breaks.

Another option here is "Exception" can be turned into a True result for "IS_CLOSED" + clear the exception but that seems like hiding an implemention issue rather than helping resolve it to me.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Oct 27, 2025

IS_CLOSED() should return -1 on error.

cmaloney
cmaloney reviewed Oct 27, 2025
View reviewed changes
cmaloney
cmaloney reviewed Oct 27, 2025
View reviewed changes
Copy link
Contributor

@cmaloney cmaloney left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also update CHECK_CLOSED to handle IS_CLOSED returning non-zero + setting an exception. In particular, if there's an exception CHECK_CLOSED should always return.

@cmaloney cmaloney changed the title gh-140650: fix SystemError in io.BufferedWriter.close when closed is not implemented gh-140650: fix SystemError in io.BufferedWriter.close when closed errors Oct 27, 2025
@inventshah
Copy link
Contributor Author

inventshah commented Oct 27, 2025

Should I still be using PyErr_Occurred() or based on @serhiy-storchaka's feedback, should we make IS_CLOSED() return -1 on exception and check the sign in both CHECK_CLOSED() and _io__Buffered_close_impl()?

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Oct 27, 2025

PyErr_Occurred() is only used when we cannot distinguish the value signalling an error from a valid value, like in PyLong_AsLong(). IS_CLOSED() returns -1 on error, 0 -- false, 1 -- true. Or so it should be.

cmaloney
cmaloney reviewed Oct 27, 2025
View reviewed changes
cmaloney
cmaloney reviewed Oct 27, 2025
View reviewed changes
efimov-mikhail
efimov-mikhail approved these changes Oct 29, 2025
View reviewed changes
Copy link
Member

@efimov-mikhail efimov-mikhail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

serhiy-storchaka
serhiy-storchaka reviewed Oct 29, 2025
View reviewed changes
Misc/NEWS.d/next/Library/2025-10-27-00-40-49.gh-issue-140650.DYJPJ9.rst Outdated
@@ -0,0 +1,2 @@
Fix an issue where closing :class:`io.BufferedWriter` could crash
if the closed attribute raised an exception.
Copy link
Member

@serhiy-storchaka serhiy-storchaka Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not accurate. Bad things happen when the value of the closed attribute cannot be converted to boolean.

Copy link
Contributor Author

@inventshah inventshah Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the "cannot be converted to a boolean" case, this patch also fixes cases where closed is defined as a managed attribute equivalent to

@property
def closed(self):
    raise Exception()

Should the news entry just focus on the first type error case, or mention fixing this case as well?

Copy link
Member

@efimov-mikhail efimov-mikhail Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add tests on this behavior too?

Copy link
Contributor Author

@inventshah inventshah Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the news to reflect both cases, and added a test for the other behavior. I wonder if we need a line mentioning that exceptions from .closed are no longer swallowed by the corresponding methods (like .write(), .flush())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@efimov-mikhail efimov-mikhail efimov-mikhail approved these changes

@cmaloney cmaloney cmaloney approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

+1 more reviewer

@ashm-dev ashm-dev ashm-dev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@inventshah @cmaloney @serhiy-storchaka @efimov-mikhail @ashm-dev